Shorten key ids in exception messages, check decoded bytes in the leak test, fix a README parameter name - #39
Merged
Merged
Conversation
… key ids already are A key pasted into an id slot in the configuration is a real mistake this library already defends against elsewhere: the same shortening that keeps markers and key ids from stored values out of logs now also applies to the constructor exceptions that repeat a configured key id, so such a paste leaks at most the first 20 characters instead of the whole key. Ids of sane length are unaffected, every existing message stays the same. The comment on `LogSafeValue` now names both sources of hostile values, stored data and misconfigured slots.
…ot just the config strings The object never stores the `prefix_hex` config strings, only the decoded raw bytes wrapped in `HiddenString`, so checking the config strings alone can't fail even if the `HiddenString` wrapping is removed and the raw keys become dumpable. The tests of the two public-key classes already check both forms; the symmetric test now does the same.
…eters example doesn't define
There was a problem hiding this comment.
Pull request overview
This PR hardens the encryption library against accidental key leakage by ensuring long/unsafe key IDs are truncated in constructor-time exception messages, improves the symmetric print_r() leak test to validate against stored decoded key bytes, and fixes a README parameter name mismatch for Nette DI usage.
Changes:
- Apply
LogSafeValue::from()to key-id-like values in several constructor exceptions so pasted keys don’t get repeated in full. - Strengthen
SymmetricKeyEncryption’s “hidden string” test to assert both the original config string and the decoded raw bytes are not present inprint_r()output. - Fix README example to use
%encryption.prefixes.*%consistently with the parameters section.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SymmetricKeyEncryptionTest.phpt | Updates leak test to check decoded key bytes and adds coverage for key-id truncation in constructor exceptions. |
| tests/AuthenticatedPublicKeyEncryptionTest.phpt | Adds a regression test ensuring long “key pasted as id” values are truncated in constructor exceptions. |
| tests/AnonymousPublicKeyEncryptionTest.phpt | Adds a regression test ensuring long “key pasted as id” values are truncated in key-pair mismatch exceptions. |
| src/Format/LogSafeValue.php | Clarifies docblock to include misconfigured key IDs as inputs that must be shortened/sanitized for exception messages. |
| src/Exceptions/KeyPairMismatchException.php | Sanitizes/truncates key id shown in mismatch message via LogSafeValue. |
| src/Exceptions/InvalidKeyRoleException.php | Sanitizes/truncates the id included in invalid-role messages via LogSafeValue. |
| src/Exceptions/InvalidKeyPrefixException.php | Sanitizes/truncates the id included in invalid/missing prefix messages via LogSafeValue. |
| src/Exceptions/InvalidKeyLengthException.php | Sanitizes/truncates the id included in invalid-length messages via LogSafeValue. |
| src/Exceptions/InvalidKeyIdException.php | Sanitizes/truncates non-empty ids included in invalid-id messages via LogSafeValue. |
| src/Exceptions/InvalidKeyEncodingException.php | Sanitizes/truncates the id included in invalid-encoding messages via LogSafeValue. |
| src/Exceptions/IncompleteKeyPairException.php | Sanitizes/truncates key id shown in incomplete pair messages via LogSafeValue. |
| README.md | Fixes service example to reference %encryption.prefixes.email% consistently with the parameters section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardening leftovers from the reviews of #38, none of which belonged in that PR:
print_r()leak test checks the decoded bytes - the object stores only decoded raw bytes, never theprefix_hexconfig strings, so the old needles could not fail even with theHiddenStringwrapping removed. The public-key class tests already check both forms; the symmetric test now does the same.%encryption.keyPrefixes.email%but the parameters section defines the group asprefixes; copying the two snippets together would fail with an undefined parameter when the container compiles.